drivers: udc_rpi_pico: replace message queue with k_events and minor fixes #87506
Conversation
jfischer-no
added
area: Drivers
area: USB
jfischer-no
force-pushed
the
pr-udc-rp_pico-rework
branch
from
ecbca9d to
1f02e60
Compare
github-actions
bot
added
the
platform: Raspberry Pi Pico
|
|
||
| k_msgq_get(&drv_msgq, &evt, K_FOREVER); | ||
| ep_cfg = udc_get_ep_cfg(dev, evt.ep); | ||
| evt = k_event_wait(&priv->events, UINT32_MAX, false, K_FOREVER); |
There was a problem hiding this comment.
Should this be either:
- The bitmask of all the defined events (e.g.
(RPI_PICO_EVT_XFER_FINISHED << 1) - 1)or similar. Or: - Should there be some checking (maybe an assert) that at the bottom of this function all events bits have been handled?
Inspection of the code says "yes, everything defined in the enum is covered below", but it feels like it'd be possible to get out of sync here.
There was a problem hiding this comment.
rpi_pico_thread_handler() is called within endless loop (static void udc_rpi_pico_thread_##n(void *dev, void *arg1, void *arg2) so there's no need for checking that all event bits have been handled.
There was a problem hiding this comment.
I'm more thinking along the lines of "if someone adds a new event, will they handle it?" but perhaps I'm suggesting an overly defensive programming style that can be countered with "if another person adds bad code... they shouldn't ", which is valid implementation strategy.
jfischer-no
force-pushed
the
pr-udc-rp_pico-rework
branch
from
1f02e60 to
3de4957
Compare
|
Rebases on main to get changes from #87480 |
drivers/usb/udc/udc_rpi_pico.c
Outdated
| struct k_event xfer_new; | ||
| struct k_event xfer_finished; |
There was a problem hiding this comment.
Actually it is not quite necessary to use k_event here. There is no need to wait for xfer_new nor xfer_finished and therefore atomic_t is enough. atomic_t is either 32-bit or 64-bit, but just 32-bit is sufficient here. See #87942 for a change in DWC2 driver.
There was a problem hiding this comment.
I commented in #87758, see also last commit here.
Set suspended state on suspend/resume ISR. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Using k_events eliminates the drawback of the queue potentially dropping messages and provides a reliable event notification mechanism. It is similar to commit c2f2d8c ("drivers: usb: udc_dwc2: Replace queue with events") Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Support VBUS state change detection and enable/disable DP pull-up according to VBUS state when pinctrl property is provided. It brings the similar functionality introduced in commit 4c0317f ("drivers: usb_dc_rpi_pico: Implemented vbus detection handling") for the legacy device controller driver. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Mark endpoint as not busy after transfer is canceled. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
For the IN endpoint, we only need to set/reset the STALL bit in the endpoint control register. To set halt on the OUT endpoint, the AVAILABLE bit must also be set, which is similar to starting a new transfer, but first any transfer in progress must be canceled. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Do not check if the tailroom is greater than or equal to MPS because the controller does not write directly to the buffer and therefore cannot write outside the buffer. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
At high throughput, the controller sometimes fails to start a new transaction. Clearing the corresponding endpoint bit in the BUFF_STATUS completion register before initiating a new transaction solves this problem. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Use uint32_t instead of k_event for event value passing and saves a few CPU cycles. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
jfischer-no
force-pushed
the
pr-udc-rp_pico-rework
branch
from
3de4957 to
d319420
Compare
|
Rebases on main to get changes from #87758 |
Replace message queue with k_events, support VBUS state change detection, fix clear/set endpoint halt.